perf(pharmacy): optimize transfer request item add — DTOs, single rate query, AJAX#21089
Merged
Merged
Conversation
…e query, AJAX add Replace full Item entity loads in pharmacy_transfer_request.xhtml with lightweight DTOs and a single-query rate lookup, eliminating the main sources of delay when adding items to a transfer request. Changes: - ItemDTO (search): add dblValue, itemTypeName, rateItemId fields + new constructor for autocomplete DTO queries - ItemRatesDTO (new): holds purchase/retail/cost rates from one query - ItemDtoConverter (new): CDI converter encodes DTO as id:type:dblValue:rateItemId - ItemController: add completeAmpAmppVmpVmppItemDtosForRequestingDepartment returning List<ItemDTO> via 4 typed constructor queries (replaces full entity load per keystroke) - PharmacyBean: add getLastRatesForItem replacing 3 separate entity-loading rate calls with a single BillItemFinanceDetails scalar query - TransferRequestController: autocomplete now uses ItemDTO + typed proxy; populateRatesOnItemSelect reduced from 3 entity loads to 1 entity load + 1 scalar query; addItem reuses cached rates; fetchBillItems adds JOIN FETCH bi.item and LEFT JOIN FETCH bi.billItemFinanceDetails to eliminate N+1 on edit path; recentToDepartments query fetches institution eagerly; three display type list getters are now lazily cached - XHTML: autocomplete bound to currentItemDto with itemDtoConverter; Add Item button changed from ajax=false (full page reload) to AJAX with targeted updates; pack-size column uses class.simpleName instead of brittle full class string comparison (also fixes Vmpp display) Closes #21062 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors transfer-request item selection to use ItemDTO and ItemRatesDTO: adds DTO models and converter, backend DTO autocomplete and rate lookup, integrates DTOs into TransferRequestController for selection and rate population, updates JSF autocomplete binding and AJAX add-item flow, and includes small finance/defaults and persistence datasource fixes. ChangesTransfer Request DTO-driven Item Selection and Rates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47c7acbb60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/webapp/pharmacy/pharmacy_transfer_request.xhtml (2)
176-178:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
txtLineCostRatefrom the AJAX update targets (src/main/webapp/pharmacy/pharmacy_transfer_request.xhtml:176-178)
txtLineCostRateis referenced in thep:ajaxupdatelist, but no component withid="txtLineCostRate"exists in this view, so the AJAX update target can’t be resolved.Suggested fix
<p:ajax event="itemSelect" listener="#{transferRequestController.populateRatesOnItemSelect}" - update="txtLineCostRate txtLineGrossRate txtLineNetValue requestTotals focusQty selDepartmentType"/> + update="txtLineGrossRate txtLineNetValue requestTotals focusQty selDepartmentType"/>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/webapp/pharmacy/pharmacy_transfer_request.xhtml` around lines 176 - 178, Remove the non-existent AJAX update target by editing the p:ajax in pharmacy_transfer_request.xhtml: locate the p:ajax with listener "#{transferRequestController.populateRatesOnItemSelect}" and remove "txtLineCostRate" from its update list so the remaining update targets (e.g., txtLineGrossRate, txtLineNetValue, requestTotals, focusQty, selDepartmentType) are valid.
211-217:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow the AJAX
processscope for “Add Item”. (src/main/webapp/pharmacy/pharmacy_transfer_request.xhtml:211-217)
process="@Form"causes the whole<h:form>(including the editablep:dataTable id="itemList"rows) to be decoded/validated on every add, so latency will scale with row count.<p:commandButton value="Add Item" icon="fas fa-plus" class="ui-button-success w-100" action="#{transferRequestController.addItem}" process="`@form`" update="itemAutoComplete txtQty txtLineGrossRate txtLineNetValue itemList requestTotals focusItem"/>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/webapp/pharmacy/pharmacy_transfer_request.xhtml` around lines 211 - 217, The Add Item button currently uses process="`@form`" which decodes/validates the entire h:form (including the editable p:dataTable id="itemList"); change the p:commandButton's process attribute to only submit the specific input components needed for addItem (for example use "`@this` itemAutoComplete txtQty txtLineGrossRate txtLineNetValue" or similar) and remove "`@form`" so the controller method addItem and validations only run for those inputs while leaving update="itemList requestTotals focusItem" unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/webapp/pharmacy/pharmacy_transfer_request.xhtml`:
- Around line 171-174: The Pack Size column is showing vt.dblValue for any
positive number; update the EL in the h:outputText so dblValue is used only when
the item type is Ampp or Vmpp. Replace the current condition "(vt.dblValue ne
null and vt.dblValue gt 0) ? vt.dblValue : 1" with a compound check that also
ensures the DTO's item-type field (e.g., vt.itemType or the actual property
holding the item type) is 'Ampp' or 'Vmpp' before using vt.dblValue, otherwise
default to 1; keep the <f:convertNumber> formatting unchanged.
---
Outside diff comments:
In `@src/main/webapp/pharmacy/pharmacy_transfer_request.xhtml`:
- Around line 176-178: Remove the non-existent AJAX update target by editing the
p:ajax in pharmacy_transfer_request.xhtml: locate the p:ajax with listener
"#{transferRequestController.populateRatesOnItemSelect}" and remove
"txtLineCostRate" from its update list so the remaining update targets (e.g.,
txtLineGrossRate, txtLineNetValue, requestTotals, focusQty, selDepartmentType)
are valid.
- Around line 211-217: The Add Item button currently uses process="`@form`" which
decodes/validates the entire h:form (including the editable p:dataTable
id="itemList"); change the p:commandButton's process attribute to only submit
the specific input components needed for addItem (for example use "`@this`
itemAutoComplete txtQty txtLineGrossRate txtLineNetValue" or similar) and remove
"`@form`" so the controller method addItem and validations only run for those
inputs while leaving update="itemList requestTotals focusItem" unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b552135-72c7-4aca-8a5c-90e31c302b8d
📒 Files selected for processing (7)
src/main/java/com/divudi/bean/common/ItemController.javasrc/main/java/com/divudi/bean/pharmacy/TransferRequestController.javasrc/main/java/com/divudi/core/converter/ItemDtoConverter.javasrc/main/java/com/divudi/core/data/dto/ItemRatesDTO.javasrc/main/java/com/divudi/core/data/dto/search/ItemDTO.javasrc/main/java/com/divudi/ejb/PharmacyBean.javasrc/main/webapp/pharmacy/pharmacy_transfer_request.xhtml
…is.git into development # Conflicts: # src/main/webapp/dataAdmin/admin_functions.xhtml Signed-off-by: buddhika <buddhika.ari@gmail.com>
… StockHistory historyType for transfer bills - TransferIssueForRequestsController: set billNetTotal and billGrossTotal to ZERO in calculateBillTotalsForTransferIssue — these were NULL causing incomplete BFD records - TransferReceiveController: same fix in the receive bill finance population block - PharmacyBean: add resolveStockHistoryType() helper and call it from both addToStockHistory overloads so StockHistory.historyType is never NULL Related: #21062 Closes #21156 (pbi/bifd value sign issue tracked separately) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
Author
|
Fix pushed — commit Three data-quality issues found during investigation of transfer TI/COOP/26/000277 / OPPHTI/460:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/resources/META-INF/persistence.xml`:
- Line 5: The persistence.xml currently hardcodes JNDI names inside the
<jta-data-source> elements (values "jdbc/coop" and "jdbc/ruhunuAudit"); replace
those hardcoded values with the CI/CD placeholders ${JDBC_DATASOURCE} for the
primary datasource and ${JDBC_AUDIT_DATASOURCE} for the audit datasource so the
pipeline variable checks pass and deployments use environment-specific JNDI
names, ensuring you update both occurrences (the two <jta-data-source> elements)
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2944baa-c5ec-4cfb-b72a-525cfb54cae6
📒 Files selected for processing (4)
src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.javasrc/main/java/com/divudi/bean/pharmacy/TransferReceiveController.javasrc/main/java/com/divudi/ejb/PharmacyBean.javasrc/main/resources/META-INF/persistence.xml
✅ Files skipped from review due to trivial changes (2)
- src/main/java/com/divudi/bean/pharmacy/TransferReceiveController.java
- src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java
…er request UI (#21089) - Gate autocomplete pack-size column by item type (Ampp/Vmpp only), defaulting to 1 for non-pack items — fixes misleading pack size display - Add IDs (panelRateInput, panelValueInput) to both conditional h:panelGroup wrappers for rate/value inputs; update all AJAX update attributes to target the panels instead of the inner component IDs, preventing JSF tree resolution failures when rates are hidden by config or privilege Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace hardcoded local JNDI names with required CI/CD variables:
- jdbc/coop → ${JDBC_DATASOURCE}
- jdbc/ruhunuAudit → ${JDBC_AUDIT_DATASOURCE}
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Itementity loads in the transfer request autocomplete with lightweightItemDTOprojections (4 typed constructor queries per keystroke instead of 1 full entity query)BillItemFinanceDetailsscalar query via newItemRatesDTOajax=false(full page reload) to AJAX with targeted component updatesJOIN FETCH bi.itemandLEFT JOIN FETCH bi.billItemFinanceDetailstofetchBillItemsbi.item.class eq 'class com.divudi...'comparison that silently broke for Hibernate proxies in edit mode (now usesclass.simpleName, and correctly handles Vmpp which was missing)institutioneagerly inrecentToDepartmentsquery to prevent lazy load inprocessTransferRequestNew files
core/data/dto/ItemRatesDTO.javacore/converter/ItemDtoConverter.javaDB calls reduced per item-add cycle
BillItementity loads for ratesitemFacade.find+ 1 × scalar rates queryitemList+requestTotalsonlyNavigation path for QA testing
fetchBillItemsJOIN FETCH path)Related issues created
ph:historycomposite component N+1 (pendingGrns— out of scope for this PR)PharmacyDepartmentStockDTOcarries fullDepartmententity (out of scope for this PR)Closes #21062
🤖 Generated with Claude Code
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores